Skip to content
This repository has been archived by the owner on Aug 11, 2020. It is now read-only.

Add subtitle addon type #7

Merged
merged 1 commit into from
Jan 20, 2017
Merged

Add subtitle addon type #7

merged 1 commit into from
Jan 20, 2017

Conversation

razzeee
Copy link
Member

@razzeee razzeee commented Jan 18, 2017

Please review.

subtitles.py done by @phate89
I feel like there is still some stuff to get rid off. So let's talk about that.

And I'm not sure if I should have replaced some of the stuff with placeholders?

@phate89
Copy link
Contributor

phate89 commented Jan 19, 2017

You mean to clean the sample? most of the code is to retrieve info from the playing. I looked to other addons and they all use almost exactly this code....so I think it's good to have it into provide something ready.
We could use json to retrieve the info needed but the cleaning code is still needed

@coveralls
Copy link

coveralls commented Jan 19, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 6dd959f6c43ea0374c1f6530735e3101f32ec479 on subtitle-type into 761f4d7 on master.

@razzeee
Copy link
Member Author

razzeee commented Jan 19, 2017

Well I just thought it might scare people away that just started doing addons.
But if you think it's worth it, I'll start to add some more code comments in the next days. Feel free to provide me with some more if you like. Here's hoping that there are really comments I could add ;)

@phate89
Copy link
Contributor

phate89 commented Jan 19, 2017

Subtitle addons are really basic stuff and they just have to respond to specific parameters.
if you open the addon with plugin://script.service.foo/?action=search or plugin://script.service.foo/?action=manualsearch&searchstring=usersearchedstring the plugin should return a list of fileitem with a link to the module to download the subtitle (for example plugin://script.service.foo/?action=download&id=1).
Then it will have to handle the call of plugin://script.service.foo/?action=download&bla=bla downloading the subtitle and returning a fileitem with url the local path of the downloaded subtitle

I understand that it's a lot of code. we could drop the code to gather info and download the subtitle but it's something that ALL subtitle addons need to chose what subtitle display and to save the file..

The alternative is to use/create some extra module to hide this so that all subtitle addons might use..

@razzeee
Copy link
Member Author

razzeee commented Jan 19, 2017

Well let's wait for some third party people like @MartijnKaijser @phil65 or @enen92 reviewing this.
I'm fine with using it as is.

@@ -0,0 +1,109 @@
import urllib2, sys, urlparse, urllib, os, unicodedata

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

SCRIPT_ID = ADDON.getAddonInfo('id')
PROFILE = xbmc.translatePath(ADDON.getAddonInfo('profile'))
TEMP = os.path.join( PROFILE, 'temp', '')
HANDLE=int(sys.argv[1])

This comment was marked as spam.

#function to retrieve parameters in a dictionary
def getParams():
if len(sys.argv) > 2:
return dict(urlparse.parse_qsl(sys.argv[2].lstrip(b'?')))

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.


def getInfo():
item = {}
item['temp'] = False

This comment was marked as spam.

@BigNoid
Copy link
Member

BigNoid commented Jan 19, 2017

Subtitles.py uses camelCase, PascalCase and the pep-8 recommendation of lowercase and underscores mixed together for functions. I think we should provide a consistent base in that regard if this is to be a template.

@razzeee
Copy link
Member Author

razzeee commented Jan 19, 2017

Run Pep8 on the Subtitles.py file, all errors fixed apart from 2-3 lines too long.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5770d9d63e70bd9cd0a07cbdb5878e712084e447 on subtitle-type into 761f4d7 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 53f7d0e on subtitle-type into 761f4d7 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 53f7d0e on subtitle-type into 761f4d7 on master.

@razzeee
Copy link
Member Author

razzeee commented Jan 20, 2017

I would like to merge this.

@phate89 could you do a followup PR with some more code comments? I feel like I understand to less about some of the stuff set here.

@phate89
Copy link
Contributor

phate89 commented Jan 20, 2017

@razzeee yes sure.. anyway what code style we want to use for variables/functions? to have a single one everywhere

@razzeee
Copy link
Member Author

razzeee commented Jan 20, 2017

@phil65 should answere that, he's far more into the python stuff

@razzeee razzeee merged commit e6be04b into master Jan 20, 2017
@razzeee razzeee deleted the subtitle-type branch January 20, 2017 22:00
@MartijnKaijser
Copy link
Member

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants